Skip to content

Conversation

@saraqzhang
Copy link
Contributor

@saraqzhang saraqzhang commented May 1, 2025

Additional modifications of fvsetup for more integrated land-atm DAS setup and configuration (on top of #327).

Contingent on:

@saraqzhang saraqzhang added the 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label May 1, 2025
@saraqzhang saraqzhang requested a review from a team as a code owner May 1, 2025 16:06
@saraqzhang saraqzhang requested review from gmao-rreichle and removed request for a team May 1, 2025 16:10
@saraqzhang
Copy link
Contributor Author

@gmao-rreichle changes here corresponding to ldasGC pr#94 draft.

@gmao-rreichle gmao-rreichle changed the title modifications in fvsetup corresponding to ldasGC pr #94 additional modifications in fvsetup corresponding on top of GEOSadas PR#327 May 2, 2025
@gmao-rreichle gmao-rreichle changed the title additional modifications in fvsetup corresponding on top of GEOSadas PR#327 additional modifications in fvsetup on top of GEOSadas PR#327 May 2, 2025
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saraqzhang, please double-check my edits in the most recent commit 03c1dda.
Also see inline comments below.

@gmao-rreichle gmao-rreichle changed the base branch from feature/saraqzhang/updatesetup4ladas to develop May 5, 2025 18:45
@gmao-rreichle gmao-rreichle marked this pull request as draft May 5, 2025 18:45
@gmao-rreichle gmao-rreichle changed the title additional modifications in fvsetup on top of GEOSadas PR#327 additional modifications in fvsetup for more integrated land-atmosphere DAS setup and configuration May 5, 2025
@gmao-rreichle gmao-rreichle added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs enhancement New feature or request and removed 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) labels May 5, 2025
@saraqzhang
Copy link
Contributor Author

updates are tested in setup of a hybrid4dnvar LADAS ( with ldas_setup updated in ldasGC PR#94)

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle update command line inputs corresponding to ldasGC PR#94. For the path info of ldas SPEC NML and MWRTM param, the options of default and user input to fvsetup are also added.

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle @rtodling the current system ( v5.42.11 + PR#352 ) passed tests of built, LADAS setup and 2-day hybrid cycling run. a tag is created LADAS_v5.42.11 .

@gmao-rreichle gmao-rreichle marked this pull request as ready for review July 1, 2025 13:11
gmao-rreichle
gmao-rreichle previously approved these changes Jul 1, 2025
@rtodling
Copy link
Collaborator

Is the change in MAPL a zero-diff change?

@weiyuan-jiang
Copy link

Is the change in MAPL a zero-diff change?

Between MAPL v2.52 and v2.57, there is no mention about non-zero-diff. So it should be zero-diff

@gmao-rreichle
Copy link
Contributor

Is the change in MAPL a zero-diff change?

Between MAPL v2.52 and v2.57, there is no mention about non-zero-diff. So it should be zero-diff

Yes, it's 0-diff. @rtodling, aren't you already using MAPL v2.56.1 (which includes the ExtData fix) in your branch? v2.57.0 only changes EASE-grid routines, which is 0-diff (and the EASE grid isn't used by ADAS anyway). But as @weiyuan-jiang said, MAPL should be 0-diff between v2.52 and v2.57

@gmao-rreichle gmao-rreichle requested review from weiyuan-jiang and removed request for weiyuan-jiang August 7, 2025 20:09
weiyuan-jiang
weiyuan-jiang previously approved these changes Aug 8, 2025
@rtodling
Copy link
Collaborator

rtodling commented Jan 15, 2026

Sara and @gmao-rreichle Just letting you know here that I pulled these changes yesterday and tried testing them, but unfortunately, I am unable to corroborate them being zero-diff. I believe the issue is in MAPL, but I have no idea why. I am getting in touch with the SI group so we can go over the differences between the two versions of MAPL in question:

GEOS-ESM/MAPL@v2.52.0...v2.57.0

@gmao-rreichle
Copy link
Contributor

Sara and @gmao-rreichle Just letting you know here that I pulled these changes yesterday and tried testing them, but unfortunately, I am unable to corroborate them being zero-diff. I believe the issue is in MAPL, but I have no idea why. I am getting in touch with the SI group so we can go over the differences between the two versions of MAPL in question:

GEOS-ESM/[email protected]

@rtodling : I'm having a hard time believing that the changes in MAPL caused any non-trivial changes. As @weiyuan-jiang said above, MAPL v2.52 and v2.57 should be zero-diff (#352 (comment)). Having said that, I did find the following comment in the MAPL v2.57 release notes (https://github.com/GEOS-ESM/MAPL/releases/tag/v2.57.0):

NOTE: There is one change that can affect some diagnostic prints made by MAPL. The per-step diagnostic print was changed from being hardcoded as AGCM Date to now trigger off of the ROOT_NAME in CAP.rc. So, if ROOT_NAME is GEOSldas, the print will be GEOSldas Date instead of AGCM Date.

Could the non-zero-diff issue that you're finding be related to something like that?

@rtodling
Copy link
Collaborator

Sara and @gmao-rreichle Just letting you know here that I pulled these changes yesterday and tried testing them, but unfortunately, I am unable to corroborate them being zero-diff. I believe the issue is in MAPL, but I have no idea why. I am getting in touch with the SI group so we can go over the differences between the two versions of MAPL in question:
GEOS-ESM/[email protected]

@rtodling : I'm having a hard time believing that the changes in MAPL caused any non-trivial changes. As @weiyuan-jiang said above, MAPL v2.52 and v2.57 should be zero-diff (#352 (comment)). Having said that, I did find the following comment in the MAPL v2.57 release notes (https://github.com/GEOS-ESM/MAPL/releases/tag/v2.57.0):

NOTE: There is one change that can affect some diagnostic prints made by MAPL. The per-step diagnostic print was changed from being hardcoded as AGCM Date to now trigger off of the ROOT_NAME in CAP.rc. So, if ROOT_NAME is GEOSldas, the print will be GEOSldas Date instead of AGCM Date.

Could the non-zero-diff issue that you're finding be related to something like that?

@gmao-rreichle What you saying above might be an issue - but only when LDAS is on, which in my testing it isn't.

I know what the issue is: the AOD analyses are not being processed correctly. Now what that is the case is still under investigation. I asked for Matt and Atanas's help - I am re-running my test case to get a dump of the yaml files to see what might be the issue.

It is possible that when Sara made the test, her baseline was not using the AOD analysis correctly - many people overlook that - which means the run might have zero-increment for AOD, in which case, she would have gotten a zero-diff result from the before and after changes here.

@gmao-rreichle
Copy link
Contributor

@rtodling : I can't speak to what exactly Sara tested and what you may be testing. But taking a step back, I can't possibly see how this PR has anything to do with the AOD analysis. There are only 2 files changed in this PR:

  • components.yaml impacts the MAPL version, which should be zero-diff (unless the documentation by the SI team is wrong).
  • fvsetup modifies the LDAS setup within the ADAS setup. If you're not running the LDAS, this should have no impact at all, and certainly not on the AOD analysis. I just went through the fvsetup changes again. The majority of the changes are within an "lana==.true." block and an LDAS-relates sub function. The only change that is not so constrained has to do with sequencing the setup of the atmos ensemble and the LDAS setup:
    if (-d $src_arcdir) {
    If the system sets up and runs, how could the changes in this PR cause non-zero diff results?

@rtodling
Copy link
Collaborator

@rtodling : I can't speak to what exactly Sara tested and what you may be testing. But taking a step back, I can't possibly see how this PR has anything to do with the AOD analysis. There are only 2 files changed in this PR:

  • components.yaml impacts the MAPL version, which should be zero-diff (unless the documentation by the SI team is wrong).
  • fvsetup modifies the LDAS setup within the ADAS setup. If you're not running the LDAS, this should have no impact at all, and certainly not on the AOD analysis. I just went through the fvsetup changes again. The majority of the changes are within an "lana==.true." block and an LDAS-relates sub function. The only change that is not so constrained has to do with sequencing the setup of the atmos ensemble and the LDAS setup:
    if (-d $src_arcdir) {

    If the system sets up and runs, how could the changes in this PR cause non-zero diff results?

@gmao-rreichle this PR relies on a change in MAPL that brings in a ton of stuff related to ExtData2G which has nothing to do with LDAS but has a large pontential to cause troubles. The problem is not in the LDAS-related knobs and changes; the problem is in MAPL and how it changes something about handling the info in the yaml files that drive the model.

@rtodling
Copy link
Collaborator

@rtodling : I can't speak to what exactly Sara tested and what you may be testing. But taking a step back, I can't possibly see how this PR has anything to do with the AOD analysis. There are only 2 files changed in this PR:

  • components.yaml impacts the MAPL version, which should be zero-diff (unless the documentation by the SI team is wrong).

  • fvsetup modifies the LDAS setup within the ADAS setup. If you're not running the LDAS, this should have no impact at all, and certainly not on the AOD analysis. I just went through the fvsetup changes again. The majority of the changes are within an "lana==.true." block and an LDAS-relates sub function. The only change that is not so constrained has to do with sequencing the setup of the atmos ensemble and the LDAS setup:

    if (-d $src_arcdir) {

    If the system sets up and runs, how could the changes in this PR cause non-zero diff results?

@gmao-rreichle this PR relies on a change in MAPL that brings in a ton of stuff related to ExtData2G which has nothing to do with LDAS but has a large pontential to cause troubles. The problem is not in the LDAS-related knobs and changes; the problem is in MAPL and how it changes something about handling the info in the yaml files that drive the model.

btw: you can be perplexed, but your can't deny the fact that I am inheriting a bug since I see it in the results.

@gmao-rreichle
Copy link
Contributor

@rtodling : Sorry, I didn't mean to imply that you're not dealing with a bug. Clearly, you are seeing non-zero-diff results. What I'm saying is just that the bug can't be related to the changes in the PR -- with the exception of the MAPL change. It may well be that the MAPL change is causing problems for the ADAS, and that this hasn't been properly tested by Sara. In my comments, I was going by the MAPL release documentation, which seems to suggest that the change was zero-diff. But perhaps this testing by the SI team doesn't include a full ADAS test.

In any case, this suggests a bigger issue. This PR needs the MAPL update because the newer MAPL includes new features that are needed by the LDAS. These features aren't related to what's causing your problem. Going forward, we probably have to choose between the following:

  1. Make ADAS work with the newer MAPL versions (which I assume has to happen eventually), or
  2. ask the SI team to create a patch to MAPL v2.52 that includes what the LDAS needs (but excludes the offending ExtData2G changes), or
  3. not merge this PR (additional modifications in fvsetup for more integrated land-atmosphere DAS setup and configuration #352).

You can probably guess that I'd love to avoid option 3. It would set back the LDAS integration even further.

I can't speak to option 1 and how relevant the non-zero-diff changes are.

Maybe it's worth inquiring with @mathomp4 about option 2? I'm somewhat pessimistic about this being an easy fix, but it shouldn't hurt to ask.

@rtodling
Copy link
Collaborator

@saraqzhang @gmao-rreichle just to keep you abreast, I am able to identify the changes that went into MAPL that cause the issue I'm having. I am working with the SI group to come up with/ a meaningful fix. The error has nothing to do with LDAS stuff but with how MAPL handles the AOD analyses file and the assimilaiton of aerosol info.

@rtodling rtodling dismissed stale reviews from weiyuan-jiang and gmao-rreichle via 449e917 January 16, 2026 18:14
@rtodling
Copy link
Collaborator

Now with MAPL v2.57.1 things work and give me zero-diff. Thanks to SI Team for patching MAPL.

Copy link
Collaborator

@rtodling rtodling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch to MAPL 2.57 gets things to work fine.

@rtodling rtodling merged commit 2046356 into develop Jan 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0 diff The changes in this pull request have verified to be zero-diff with the target branch. enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants